Skip to content

[LWDM] chore(aggregated-assets): move domain layer behind shims (LIVE-35226) - #20345

Open
LucasWerey wants to merge 9 commits into
developfrom
chore/aggregated-assets-LIVE-35226-move-domain-layer
Open

[LWDM] chore(aggregated-assets): move domain layer behind shims (LIVE-35226)#20345
LucasWerey wants to merge 9 commits into
developfrom
chore/aggregated-assets-LIVE-35226-move-domain-layer

Conversation

@LucasWerey

@LucasWerey LucasWerey commented Aug 3, 2026

Copy link
Copy Markdown
Member

✅ Checklist

  • npx changeset was attached.
  • Covered by automatic tests. The characterization tests from LIVE-35224 are the gate: they pass unmodified through the shims — 21 suites, 245 tests. A new test pins the frozen reducerPath.
  • Impact of the changes:
    - Behaviour-neutral by design. Zero consumer files change; all 82 consumers keep working through one-line re-export shims at the old paths.
    - No runtime behaviour is altered — no symbol renames, no signature changes, no zod, no endpoint restructuring.
    - QA focus if anything is spot-checked: Market (price / 24h change badges), Portfolio (distribution), and the asset/network selectors — these are the surfaces fed by the relocated cache selectors.

📝 Description

Problem. libs/ledger-live-common/src/dada-client is being migrated into the DDD layers under LIVE-35223. The packages were scaffolded empty in #20285; this PR moves the domain layer into them.

Solution. Relocate the entity types and the API layer, leaving shims behind so consumer retargeting is a separate, independently revertable step (LIVE-35228 / 35229 / 35230).

Package Now contains
@domain/entity-aggregated-asset CryptoAssetMeta
@domain/entity-interest-rate InterestRate, ApyType
@domain/api-aggregated-assets schema.ts (wire format), types.ts, transforms.ts, api.ts, errors.ts, internals/, 3 mocks via ./mock* exports

11 files under dada-client became one-line re-exports. libs/ledger-live-common gains the three packages as workspace deps — it already declared 8 @domain/* deps, so this is the established pattern.

Decisions worth reviewing

  • transforms.ts is split out of api.ts. convertApiAssets + transformAssetsResponse are now separately testable, matching the domain/api/market-sentiment precedent where transforms.ts is where the wire schema meets the entity.
  • NetworkInfo and CurrenciesOrder land in the api package, not an entity. This corrects the original scoping. A network is a chain, already modelled by @domain/entity-currency-crypto, so NetworkInfo stays a wire type resolving to that rather than duplicating the concept. CurrenciesOrder is { key, order, metaCurrencyIds } — server sort metadata, not a business object. Agreed with @gre and Yoann; see DADA DDD compliant.
  • The last boundary violation is gone. dadaIdToMarketId() and the market item type are copied into internals/market.ts rather than imported, because a domain/* package must not import legacy libs/*. The drift risk is documented in the file and the README: PartialMarketItemResponse is Partial<MarketItemResponse>, so every field is optional and future divergence will never produce a type error. Carries a TODO pointing at a future market entity.
  • reducerPath stays the literal "assetsDataApi", now pinned by a test. createCurrencyDataSelector hand-scans that key as a string and three Storybook files preload it, so a rename would silently return undefined for every market and interest-rate lookup with no type error. Any rename belongs to LIVE-35301.
  • internals/ is exported, despite the name. chunkCurrencyIds, mergeAssetsDataPages and dadaIdToMarketId are needed by the platform-layer hooks and assetDiscovery in LIVE-35227, so they are public with a comment explaining why. Flagging since the directory name now slightly overstates.

What was deliberately not touched

  • The lenient conversion paths in convertApiAssets — unconvertible tokens are still silently dropped, and cryptos missing from the local CAL are still synthesised rather than dropped. That leniency is load-bearing and now carries a comment saying so.
  • getChunkedAssetsData still succeeds if any chunk resolves. Portfolio distribution depends on it.
  • An asset with id: "" still fails the entire query. Pinned as current behaviour; fixed in LIVE-35233.
  • No endpoint restructuring — the injectEndpoints per-use-case split is LIVE-35301.

Verification

  • Characterization tests pass unmodified: 21 suites, 245 tests. Their stack traces now resolve into domain/api/aggregated-assets/src/transforms.ts, which confirms they exercise the moved code through the shims rather than passing vacuously.
  • All four packages typecheck; libs/ledger-live-common typechecks with 0 errors.
  • nx show projects lists all four; oxfmt and oxlint clean; commitlint --from origin/develop passes.
  • Scope confirmed: nothing outside dada-client, the three target packages, and the two manifests is touched.

Pre-existing issues encountered, not caused by this PR

  • Desktop jest cannot run locallylibs/ui/packages/react/lib is unbuilt, so all 62 suites fail in tests/jestSetup.js on @ledgerhq/react-ui/assets/fonts. I verified consumers still resolve via desktop typecheck instead.
  • One desktop typecheck error at dada-client/entities/selectorUtils.ts:34 (untyped cache scan). That file is byte-identical to develop and imports only @reduxjs/toolkit, so the error is pre-existing — and a fitting demonstration of why this code is guarded by tests rather than types.
  • libs/asset-detail reports 118 implicit-any errors, all in unrelated live-common files (account/formatters.ts, hw/deviceAccess.ts, exchange, bridge). None mention this code.

❓ Context


🧐 Checklist for the PR Reviewers

  • The code aligns with the requirements described in the linked JIRA or GitHub issue.
  • The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
  • There are no undocumented trade-offs, technical debt, or maintainability issues.
  • The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
  • Any new dependencies have been justified and documented.
  • Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

Copilot AI lite review requested due to automatic review settings August 3, 2026 13:50
@live-github-bot live-github-bot Bot added the common Has changes in live-common label Aug 3, 2026
@LucasWerey LucasWerey changed the title chore(aggregated-assets): move the domain layer behind shims (LIVE-35226) chore(aggregated-assets): move domain layer behind shims (LIVE-35226) Aug 3, 2026
@live-github-bot live-github-bot Bot changed the title chore(aggregated-assets): move domain layer behind shims (LIVE-35226) [LWDM] chore(aggregated-assets): move the domain layer behind shims (LIVE-35226) Aug 3, 2026
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Web Tools Build Status

Build Status Deployment
Web Tools Build ✅ Deployed https://web-tools-pa8ixgmh3-ledger-hq-prd.vercel.app
Native Storybook Build ✅ Deployed https://native-ui-storybook-pk3mdx0is-ledger-hq-prd.vercel.app
React Storybook Build ✅ Deployed https://react-ui-storybook-p30myi3dm-ledger-hq-prd.vercel.app

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Relocates the legacy libs/ledger-live-common/src/dada-client “domain layer” into the DDD domain/entity/* and domain/api/* packages, while keeping the previous import paths working via one-line re-export shims in live-common. This supports the ongoing DDD migration without forcing consumer rewrites in the same step.

Changes:

  • Introduces @domain/entity-aggregated-asset, @domain/entity-interest-rate, and @domain/api-aggregated-assets implementations (types, RTK Query API, transforms, errors, and internals).
  • Replaces the legacy dada-client implementation files with re-export shims pointing to the new domain packages.
  • Updates workspace dependencies (live-common + lockfile) and adds a small test to pin the frozen assetsDataApi.reducerPath.

Reviewed changes

Copilot reviewed 34 out of 35 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
pnpm-lock.yaml Adds new workspace importers; also contains additional lockfile churn.
libs/ledger-live-common/src/dada-client/utils/mergeAssetsDataPages.ts Shim re-export to domain API implementation.
libs/ledger-live-common/src/dada-client/utils/errorUtils.ts Shim re-export of error utilities/types from domain API.
libs/ledger-live-common/src/dada-client/utils/deepMergeCryptoAssets.ts Shim re-export to domain API implementation.
libs/ledger-live-common/src/dada-client/utils/chunkCurrencyIds.ts Shim re-export of chunking util/type from domain API.
libs/ledger-live-common/src/dada-client/types/trend.ts Moves ApyType export to the interest-rate entity package.
libs/ledger-live-common/src/dada-client/state-manager/types.ts Shim re-export of API types/enums/constants from domain API.
libs/ledger-live-common/src/dada-client/state-manager/api.ts Shim re-export of the full aggregated-assets API surface.
libs/ledger-live-common/src/dada-client/mocks/stocks.mock.ts Shim re-export of stocks mock from domain API subpath export.
libs/ledger-live-common/src/dada-client/mocks/stablecoins.mock.ts Shim re-export of stablecoins mock from domain API subpath export.
libs/ledger-live-common/src/dada-client/entities/index.ts Shims entity/wire types to the new DDD packages.
libs/ledger-live-common/src/dada-client/__mocks__/assets.mock.ts Shim re-export of assets mocks from domain API mock export.
libs/ledger-live-common/package.json Adds the new @domain/* packages as dependencies (and tidies ordering).
domain/entity/interest-rate/src/types.ts Introduces ApyType union in entity package.
domain/entity/interest-rate/src/schema.ts Adds interest-rate type definition (currently as TS interface).
domain/entity/interest-rate/src/index.ts Exposes interest-rate exports from the entity package.
domain/entity/aggregated-asset/src/schema.ts Adds aggregated-asset meta type definition (currently as TS interface).
domain/entity/aggregated-asset/src/index.ts Exposes aggregated-asset exports from the entity package.
domain/api/aggregated-assets/tsconfig.json Aligns TS libs/types with other domain/api packages (DOM + node types).
domain/api/aggregated-assets/src/types.ts Defines API-facing types/enums/params used by RTK Query and consumers.
domain/api/aggregated-assets/src/transforms.ts Extracts response transforms + wire→entity currency conversion.
domain/api/aggregated-assets/src/stocks.mock.ts Moves stocks mock into the domain API package.
domain/api/aggregated-assets/src/stablecoins.mock.ts Moves stablecoins mock into the domain API package.
domain/api/aggregated-assets/src/schema.ts Defines the raw wire contract shapes (interfaces).
domain/api/aggregated-assets/src/internals/mergeAssetsDataPages.ts Internal page-merging helper relocated into the domain API package.
domain/api/aggregated-assets/src/internals/market.ts Internal market typing + dadaIdToMarketId copied to avoid legacy imports.
domain/api/aggregated-assets/src/internals/deepMergeCryptoAssets.ts Internal deep-merge helper relocated into the domain API package.
domain/api/aggregated-assets/src/internals/chunkCurrencyIds.ts Internal chunking helper relocated into the domain API package.
domain/api/aggregated-assets/src/index.ts Barrel exports for the new domain API package (incl. selected internals + mocks).
domain/api/aggregated-assets/src/errors.ts Error utilities moved into the domain API package.
domain/api/aggregated-assets/src/assetsData.mock.ts Moves aggregated-assets mock fixtures into the domain API package.
domain/api/aggregated-assets/src/api.ts RTK Query API relocated into the domain API package (incl. frozen reducerPath comment).
domain/api/aggregated-assets/src/api.test.ts Adds test pinning assetsDataApi.reducerPath.
domain/api/aggregated-assets/package.json Defines exports (incl. mock subpaths), deps, and peers for the domain API package.
.changeset/quiet-comets-relocate.md Changeset documenting the relocation + live-common patch bump.
Files not reviewed (1)
  • pnpm-lock.yaml: Generated file

Comment on lines +1 to +5
/** An interest rate attached to one currency. */
export interface InterestRate {
/** Currency identifier */
currencyId: string;
/** Interest rate value */

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right — fixed in 22b1c04.

domain/entity/README.md is explicit: "Use zod for schema definition", with schema.ts and schema.test.ts listed as required, and "The schema is always required — it is the canonical data model and the primary reason the package exists." Shipping plain interfaces diverged from that.

Now follows the domain/entity/market-sentiment layout: schema.ts holds the Zod schema, types.ts infers the type via z.infer, schema.test.ts covers validation, and the barrel re-exports both.

Worth noting why this was safe to do inside a behaviour-neutral relocation, since my initial reasoning for deferring it was wrong. I had assumed adopting Zod meant adopting branded types, which would force a .parse() at the boundary and break consumers building these shapes from raw JSON. But a plain z.object() without .brand() infers a structurally identical type — so the schema can exist and be exported without anything being obliged to parse yet. Confirmed: libs/ledger-live-common still typechecks with 0 errors and the characterization tests pass unmodified (21 suites, 245 tests).

Applying .parse() at the api boundary remains LIVE-35232, which is where the drop-invalid-item semantics get decided.

Two loosenesses are deliberate and now documented and tested rather than tightened:

  • InterestRate.type stays z.string() rather than the ApyType enum. DADA genuinely sends kinds outside "NRR" | "APY" | "APR" and useInterestRatesByCurrencies drops them — behaviour pinned in test(dada-client): characterize the untested surface (LIVE-35224) #20276. Narrowing here would claim a guarantee the wire doesn't give.
  • fetchAt stays z.string() rather than DateTimeIsoSchema. I checked: it has zero production reads — every occurrence is a mock, fixture or Storybook story. Validating a field nobody consumes could only discard otherwise-good rates.

Comment on lines +1 to +5
/**
* An aggregated asset: one logical asset grouping several per-network currencies.
*
* `assetsIds` maps a network id to the currency id that represents this asset on that network,
* which is what makes it "aggregated" rather than a single currency.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right — fixed in 22b1c04.

domain/entity/README.md is explicit: "Use zod for schema definition", with schema.ts and schema.test.ts listed as required, and "The schema is always required — it is the canonical data model and the primary reason the package exists." Shipping plain interfaces diverged from that.

Now follows the domain/entity/market-sentiment layout: schema.ts holds the Zod schema, types.ts infers the type via z.infer, schema.test.ts covers validation, and the barrel re-exports both.

Worth noting why this was safe to do inside a behaviour-neutral relocation, since my initial reasoning for deferring it was wrong. I had assumed adopting Zod meant adopting branded types, which would force a .parse() at the boundary and break consumers building these shapes from raw JSON. But a plain z.object() without .brand() infers a structurally identical type — so the schema can exist and be exported without anything being obliged to parse yet. Confirmed: libs/ledger-live-common still typechecks with 0 errors and the characterization tests pass unmodified (21 suites, 245 tests).

Applying .parse() at the api boundary remains LIVE-35232, which is where the drop-invalid-item semantics get decided.

Two loosenesses are deliberate and now documented and tested rather than tightened:

  • InterestRate.type stays z.string() rather than the ApyType enum. DADA genuinely sends kinds outside "NRR" | "APY" | "APR" and useInterestRatesByCurrencies drops them — behaviour pinned in test(dada-client): characterize the untested surface (LIVE-35224) #20276. Narrowing here would claim a guarantee the wire doesn't give.
  • fetchAt stays z.string() rather than DateTimeIsoSchema. I checked: it has zero production reads — every occurrence is a mock, fixture or Storybook story. Validating a field nobody consumes could only discard otherwise-good rates.

Comment thread pnpm-lock.yaml
Comment on lines 23121 to 23125
metro-react-native-babel-preset: '*'
react: 19.1.4
react-dom: 19.1.4
webpack: ^5.89.0
webpack: '*'
peerDependenciesMeta:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 22b1c04 — the lockfile diff is now 48 insertions, 0 deletions, containing only the three new workspace importers.

You were right to keep pushing on this. I'd previously concluded the churn was unavoidable pnpm re-resolution triggered by the workspace set changing, and said so on an earlier PR. That was wrong. I tested it properly this time: reverting the two peer-range rewrites by hand and then running pnpm install --frozen-lockfile passes, which proves they were cosmetic rather than required.

The two reverted:

  • any-observable@0.3.0: rxjs: ^5.5.10 (pnpm had rewritten it to '*')
  • @storybook/addon-react-native-web@0.0.29: webpack: ^5.89.0 (rewritten to '*')

Neither package is touched by this PR, and --frozen-lockfile is what CI enforces, so keeping develop's values is both correct and reviewable.

For the record on the other half of your note: packageManager pins pnpm@10.24.0 and that is the version used, so the rewrites weren't a version mismatch — pnpm just normalises those ranges opportunistically whenever it rewrites the file.

Comment thread pnpm-lock.yaml
Comment on lines 25464 to +25468
@@ -25425,7 +25465,7 @@ packages:
resolution: {integrity: sha512-/FQM1EDkTsf63Ub2C6O7GuYFDsSXUwsaZDurV0np41ocwq0jthUAYCmhBX9f+KwlaCgIuWyr/4WlUQUBfKfZog==}
engines: {node: '>=6'}
peerDependencies:
rxjs: ^5.5.10
rxjs: '*'

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 22b1c04 — the lockfile diff is now 48 insertions, 0 deletions, containing only the three new workspace importers.

You were right to keep pushing on this. I'd previously concluded the churn was unavoidable pnpm re-resolution triggered by the workspace set changing, and said so on an earlier PR. That was wrong. I tested it properly this time: reverting the two peer-range rewrites by hand and then running pnpm install --frozen-lockfile passes, which proves they were cosmetic rather than required.

The two reverted:

  • any-observable@0.3.0: rxjs: ^5.5.10 (pnpm had rewritten it to '*')
  • @storybook/addon-react-native-web@0.0.29: webpack: ^5.89.0 (rewritten to '*')

Neither package is touched by this PR, and --frozen-lockfile is what CI enforces, so keeping develop's values is both correct and reviewable.

For the record on the other half of your note: packageManager pins pnpm@10.24.0 and that is the version used, so the rewrites weren't a version mismatch — pnpm just normalises those ranges opportunistically whenever it rewrites the file.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Rsdoctor Bundle Diff Analysis

Found 7 projects in monorepo, 7 projects with changes.

📊 Quick Summary
Project Total Size Change
desktop-main 2.3 MB -
desktop-preloader 7.1 KB -
desktop-renderer 80.7 MB -
desktop-webviewDappPreloader 0 B -
desktop-webviewPreloader 200.0 B -
desktop-workers 36.8 KB -
mobile 262.0 MB -
📋 Detailed Reports (Click to expand)

📁 desktop-main

Path: rsdoctor/desktop-main/rsdoctor-data.json

⚠️ No baseline data found - Unable to perform comparison analysis

Metric Current Baseline Change
📊 Total Size 2.3 MB - -
📄 JavaScript 2.2 MB - -
🎨 CSS 0 B - -
🌐 HTML 0 B - -
📁 Other Assets 135.3 KB - -

📁 desktop-preloader

Path: rsdoctor/desktop-preloader/rsdoctor-data.json

⚠️ No baseline data found - Unable to perform comparison analysis

Metric Current Baseline Change
📊 Total Size 7.1 KB - -
📄 JavaScript 5.3 KB - -
🎨 CSS 0 B - -
🌐 HTML 0 B - -
📁 Other Assets 1.8 KB - -

📁 desktop-renderer

Path: rsdoctor/desktop-renderer/rsdoctor-data.json

⚠️ No baseline data found - Unable to perform comparison analysis

Metric Current Baseline Change
📊 Total Size 80.7 MB - -
📄 JavaScript 29.4 MB - -
🎨 CSS 183.2 KB - -
🌐 HTML 1.8 KB - -
📁 Other Assets 51.2 MB - -

📁 desktop-webviewDappPreloader

Path: rsdoctor/desktop-webviewDappPreloader/rsdoctor-data.json

⚠️ No baseline data found - Unable to perform comparison analysis

Metric Current Baseline Change
📊 Total Size 0 B - -
📄 JavaScript 0 B - -
🎨 CSS 0 B - -
🌐 HTML 0 B - -
📁 Other Assets 0 B - -

📁 desktop-webviewPreloader

Path: rsdoctor/desktop-webviewPreloader/rsdoctor-data.json

⚠️ No baseline data found - Unable to perform comparison analysis

Metric Current Baseline Change
📊 Total Size 200.0 B - -
📄 JavaScript 200.0 B - -
🎨 CSS 0 B - -
🌐 HTML 0 B - -
📁 Other Assets 0 B - -

📁 desktop-workers

Path: rsdoctor/desktop-workers/rsdoctor-data.json

⚠️ No baseline data found - Unable to perform comparison analysis

Metric Current Baseline Change
📊 Total Size 36.8 KB - -
📄 JavaScript 36.8 KB - -
🎨 CSS 0 B - -
🌐 HTML 0 B - -
📁 Other Assets 0 B - -

📁 mobile

Path: rsdoctor/mobile/rsdoctor-data.json

⚠️ No baseline data found - Unable to perform comparison analysis

Metric Current Baseline Change
📊 Total Size 262.0 MB - -
📄 JavaScript 110.7 MB - -
🎨 CSS 0 B - -
🌐 HTML 0 B - -
📁 Other Assets 151.3 MB - -

Generated by Rsdoctor GitHub Action

LucasWerey added a commit that referenced this pull request Aug 3, 2026
Addresses review on #20345.

domain/entity/README.md requires each entity package to define its canonical
model as a Zod schema: 'Use zod for schema definition', with schema.ts and
schema.test.ts listed as required. Both new entity packages shipped plain
TypeScript interfaces instead, which diverged from that convention.

Converted to the market-sentiment layout: schema.ts holds the Zod schema,
types.ts infers the type, schema.test.ts covers validation, barrel re-exports
both.

Behaviour-neutral. The schemas use plain z.object without .brand(), so z.infer
produces types structurally identical to the previous interfaces - live-common
still typechecks with zero errors and the characterization tests pass unchanged.
Nothing calls .parse() at the api boundary yet; that is LIVE-35232.

Two deliberate loosenesses are documented and tested rather than tightened:
InterestRate.type stays a plain string because DADA sends kinds outside ApyType
and consumers drop them, and fetchAt stays a plain string rather than
DateTimeIsoSchema because nothing reads it, so validating the format could only
discard otherwise-good rates.

Also fixes the Domain Test CI failure: the job installs only ./domain/** and
./shared/**, so @shared/env's transitive @ledgerhq/live-env was unresolvable.
api.test.ts now mocks @shared/env with a factory so the real module is never
required.

And trims pnpm-lock.yaml to the three new importers only. The peer-range
rewrites pnpm emitted for any-observable and @storybook/addon-react-native-web
were cosmetic - reverting them keeps 'pnpm install --frozen-lockfile' passing,
so the diff is now purely additive.
Copilot AI review requested due to automatic review settings August 3, 2026 14:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 39 out of 40 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Generated file
Suppressed comments (3)

domain/api/aggregated-assets/src/api.ts:7

  • FetchBaseQueryError, FetchBaseQueryMeta, and QueryReturnValue are types, but they’re imported as values. With SWC/Jest (and in general ESM builds), importing type-only names as runtime exports can lead to runtime import errors or unnecessary runtime dependencies. Align with other domain/api packages by marking these as type imports.
import {
  createApi,
  fetchBaseQuery,
  FetchBaseQueryError,
  FetchBaseQueryMeta,
  QueryReturnValue,
} from "@reduxjs/toolkit/query/react";

domain/api/aggregated-assets/src/errors.ts:1

  • FetchBaseQueryError is only used as a type (in a type guard), so it should be imported as a type-only import. This avoids emitting a runtime named import for a type-only symbol (which may not exist at runtime) and matches the pattern used in other domain/api packages.
import { FetchBaseQueryError } from "@reduxjs/toolkit/query";

domain/api/aggregated-assets/src/api.ts:182

  • This comment documents a frozen workaround (hard-coded reducerPath). Per repo comment guidance, workaround comments should include a link to the tracking ticket so the rationale doesn’t go stale when the migration continues.
  /*
   * FROZEN. createCurrencyDataSelector hand-scans state.assetsDataApi.queries by string, and
   * Storybook stories preload this exact key. Renaming it silently returns undefined for every
   * market and interest-rate lookup, with no type error.
   */

@LucasWerey LucasWerey changed the title [LWDM] chore(aggregated-assets): move the domain layer behind shims (LIVE-35226) [LWDM] chore(aggregated-assets): move domain layer behind shims (LIVE-35226) Aug 3, 2026
@LucasWerey

Copy link
Copy Markdown
Member Author

Quality Gate Failed Quality Gate failed

Failed conditions 11.3% Coverage on New Code (required ≥ 80%) 3 New Code Smells (required ≤ 1)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Won't be fixed here as we are only moving things

@LucasWerey
LucasWerey marked this pull request as ready for review August 4, 2026 08:38
@LucasWerey
LucasWerey requested review from a team as code owners August 4, 2026 08:38
/** Type of rate (NRR, APR, APY, etc.) — intentionally wider than ApyType, see above */
type: z.string(),
/** Timestamp when the rate was fetched */
fetchAt: z.string(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the type but is it compatible with DatetimeIso from @shared/schema-primitive package?

@LucasWerey LucasWerey Aug 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction on my "Done" above — I reverted it in 878ad8f.

DateTimeIsoSchema is the right type (DADA sends RFC 3339, and all four timestamp shapes it produces pass the schema, 6-digit fractional seconds included). The problem was downstream: branding fetchAt broke tsc in three characterization fixtures, which had to be rebuilt through InterestRateSchema.parse(...) to compile.

Those fixtures are the safety net for the whole epic and the deal is that they pass unmodified — spending that on a nice-to-have inside a move-only PR is a bad trade, so it goes to LIVE-35232, which brands assetsIds and already expects to touch consumers.

Worth recording that the reason it looked cheap at first is that CI typechecks live-common with -p src/tsconfig.json --customConditions node, and the root config I checked first reported zero errors.

/** An interest rate attached to one currency. */
export const InterestRateSchema = z.object({
/** Currency identifier */
currencyId: z.string(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the constraint but can't it be constrained to currency ids branded types or is it completly different types?

@LucasWerey LucasWerey Aug 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done; I used an union of both brands rather than CryptoCurrencyIdSchema

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same story as the fetchAt thread — reverted in 878ad8f, moved to LIVE-35232.

To actually answer the question though, since it changes what we implement there: it is not CryptoCurrencyIdSchema. DADA keys interestRates by both crypto ids (ethereum) and token ids (ethereum/erc20/usd_tether), so it needs z.union([CryptoCurrencyIdSchema, TokenCurrencyIdSchema]). That is the same union assetsIds needs on its value side, so the two are one piece of work — another reason to do them together rather than half here.

/** Asset display name */
name: z.string(),
/** Map of network IDs to their corresponding asset IDs */
assetsIds: z.record(z.string(), z.string()),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keys can't be related to currency ids branded types too or different ids?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do it in this task LIVE-35232

Comment thread domain/api/aggregated-assets/src/api.ts Outdated
);
}

export const assetsDataApi = createApi({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use injectEndpoints. Empty api should be registered on @shared/api-services

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've a dedicated Jira task for it: LIVE-35301
As the task here is just to move things

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correcting myself here — this is done, not deferred. I pointed at LIVE-35301 before checking, and @shared/api-services already existed (its README even names this migration). Shipped in d4f207e.

shared/api-services/src/services/dada/ now holds the endpoint-less base with the frozen reducerPath, and domain/api/aggregated-assets does dadaApi.enhanceEndpoints({ addTagTypes }).injectEndpoints(...). One reducer, one middleware, one cache slice — which createCurrencyDataSelector depends on, since it hand-scans state.assetsDataApi.queries by string.

Two things genuinely left, both needing consumer changes so out of a move-only PR:

  • the base URL is still resolved in domain/api via getEnv("DADA_API_*"), where it should reach the service as extraArgument
  • the apps still register the use-case api rather than dadaApi

LIVE-35301 is now scoped to splitting the single injectEndpoints call into three per-use-case modules, which is the part that actually needed its own revert.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit messy. It should be more something like export * from "file" all the rest you don't want to expose should go on internals files or dir.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Comment on lines +46 to +51
export { chunkCurrencyIds } from "./internals/chunkCurrencyIds";
export type { CurrencyIdChunks } from "./internals/chunkCurrencyIds";
export { deepMergeCryptoAssets } from "./internals/deepMergeCryptoAssets";
export { mergeAssetsDataPages } from "./internals/mergeAssetsDataPages";
export { dadaIdToMarketId } from "./internals/market";
export type { MarketItemResponse, PartialMarketItemResponse } from "./internals/market";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Files under internals should not be exposed

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment thread domain/api/aggregated-assets/src/api.ts Outdated
Comment on lines +28 to +53
function allSettled<T>(promises: Promise<T>[]): Promise<SettledResult<T>[]> {
return Promise.all(
promises.map(p =>
p
.then(value => ({ status: "fulfilled" as const, value }))
.catch(reason => ({ status: "rejected" as const, reason })),
),
);
}

function assertDadaApiUrl(url: URL): void {
if (!ALLOWED_DADA_HOSTS.has(url.hostname)) {
throw new Error(`Blocked request to untrusted host: ${url.hostname}`);
}
}

function emptyAssetsData(): AssetsData {
return {
cryptoAssets: {},
networks: {},
cryptoOrTokenCurrencies: {},
interestRates: {},
markets: {},
currenciesOrder: { metaCurrencyIds: [], key: "", order: "" },
};
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could go on internals and could be tested there

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment thread domain/api/aggregated-assets/src/api.ts Outdated
Comment on lines +86 to +163
function resolveBaseUrl(queryArg: { isStaging?: boolean }): string {
return queryArg.isStaging ? getEnv("DADA_API_STAGING") : getEnv("DADA_API_PROD");
}

async function fetchAssetsPage(
baseUrl: string,
queryArg: GetAssetsDataParams,
): Promise<AssetsData> {
const params = buildAssetsQueryParams(queryArg);
const url = new URL(`${baseUrl}/assets`);
for (const [key, value] of Object.entries(params)) {
if (value !== undefined) {
url.searchParams.set(key, Array.isArray(value) ? value.join(",") : String(value));
}
}

assertDadaApiUrl(url);
const response = await fetch(url.toString());

if (!response.ok) {
throw new Error(`DADA fetch failed: ${response.status} ${response.statusText}`);
}

const raw: RawApiResponse = await response.json();
const enrichedCryptoOrTokenCurrencies = convertApiAssets(raw.cryptoOrTokenCurrencies);

return {
...raw,
cryptoOrTokenCurrencies: enrichedCryptoOrTokenCurrencies,
};
}

async function collectAllByCategory(
queryArg: GetAssetsByCategoryParams,
extract: (data: RawApiResponse) => string[],
): Promise<QueryReturnValue<string[], FetchBaseQueryError, FetchBaseQueryMeta | undefined>> {
try {
const baseUrl = queryArg.isStaging ? getEnv("DADA_API_STAGING") : getEnv("DADA_API_PROD");
const collected: string[] = [];
let cursor: string | undefined;

do {
const url = new URL(`${baseUrl}/assets`);
url.searchParams.set("categories", queryArg.category);
url.searchParams.set("product", queryArg.product);
url.searchParams.set("pageSize", "100");
url.searchParams.set("minVersion", queryArg.version);
if (cursor) {
url.searchParams.set("cursor", cursor);
}

assertDadaApiUrl(url);
const response = await fetch(url.toString());

if (!response.ok) {
return {
error: {
status: response.status,
data: `Failed to fetch assets by category: ${response.statusText}`,
},
};
}

const data: RawApiResponse = await response.json();
collected.push(...extract(data));
cursor = response.headers.get("x-ledger-next") || undefined;
} while (cursor);

return { data: collected };
} catch (error) {
return {
error: {
status: "FETCH_ERROR",
error: error instanceof Error ? error.message : "Unknown error",
},
};
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could go to internals too

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment thread domain/api/aggregated-assets/src/api.ts Outdated
Comment on lines +165 to +175
export function fetchAllAssetsByCategory(queryArg: GetAssetsByCategoryParams) {
return collectAllByCategory(queryArg, data =>
Object.values(data.cryptoAssets).map(a => a.ticker),
);
}

export function fetchAllAssetCurrencyIdsByCategory(queryArg: GetAssetsByCategoryParams) {
return collectAllByCategory(queryArg, data =>
Object.values(data.cryptoAssets).flatMap(meta => Object.values(meta.assetsIds)),
);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe better to move them into an accessors.ts file

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Relocate the dada-client entity types and API layer into the aggregated-assets
DDD packages (LIVE-35226, epic LIVE-35223). Behaviour-neutral: zero consumer
files change, all 82 consumers keep working through one-line shims at the old
paths.

Pure relocation - no symbol renames, no signature changes, no zod, and no
endpoint restructuring. The per-use-case injectEndpoints split is LIVE-35301.

  entity-aggregated-asset  CryptoAssetMeta
  entity-interest-rate     InterestRate, ApyType
  api-aggregated-assets    wire schema, transforms, api, internals, errors, mocks

transforms.ts is split out of api.ts so the wire->entity boundary is separately
testable, matching the market-sentiment precedent.

NetworkInfo and CurrenciesOrder land in the api package rather than an entity: a
network IS a chain, already modelled by entity-currency-crypto, and
CurrenciesOrder is server sort metadata. Both were entities in the original
scoping; the naming review corrected that.

Breaks the last boundary violation. dadaIdToMarketId and the market item type
are copied from libs/ledger-live-common/src/market into internals/market.ts,
since a domain/* package cannot import legacy libs/*. Drift risk is documented
there and in the README: every field is optional, so divergence will never
produce a type error.

reducerPath stays the literal "assetsDataApi" and is now pinned by a test.
createCurrencyDataSelector hand-scans that key as a string and Storybook stories
preload it, so a rename would silently return undefined for every market and
interest-rate lookup with no type error.

The characterization tests from LIVE-35224 pass unmodified: 21 suites, 245
tests.
Addresses review on #20345.

domain/entity/README.md requires each entity package to define its canonical
model as a Zod schema: 'Use zod for schema definition', with schema.ts and
schema.test.ts listed as required. Both new entity packages shipped plain
TypeScript interfaces instead, which diverged from that convention.

Converted to the market-sentiment layout: schema.ts holds the Zod schema,
types.ts infers the type, schema.test.ts covers validation, barrel re-exports
both.

Behaviour-neutral. The schemas use plain z.object without .brand(), so z.infer
produces types structurally identical to the previous interfaces - live-common
still typechecks with zero errors and the characterization tests pass unchanged.
Nothing calls .parse() at the api boundary yet; that is LIVE-35232.

Two deliberate loosenesses are documented and tested rather than tightened:
InterestRate.type stays a plain string because DADA sends kinds outside ApyType
and consumers drop them, and fetchAt stays a plain string rather than
DateTimeIsoSchema because nothing reads it, so validating the format could only
discard otherwise-good rates.

Also fixes the Domain Test CI failure: the job installs only ./domain/** and
./shared/**, so @shared/env's transitive @ledgerhq/live-env was unresolvable.
api.test.ts now mocks @shared/env with a factory so the real module is never
required.

And trims pnpm-lock.yaml to the three new importers only. The peer-range
rewrites pnpm emitted for any-observable and @storybook/addon-react-native-web
were cosmetic - reverting them keeps 'pnpm install --frozen-lockfile' passing,
so the diff is now purely additive.
Copilot AI review requested due to automatic review settings August 4, 2026 09:33
@LucasWerey
LucasWerey force-pushed the chore/aggregated-assets-LIVE-35226-move-domain-layer branch from 22b1c04 to dbe7fc6 Compare August 4, 2026 09:33
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

⚠️ E2E tests are required

Changes detected require e2e testing before merge (even before asking for any review).

🖥️ Desktop

-> Run Desktop E2E

  • Select "Run workflow"
  • Branch: chore/aggregated-assets-LIVE-35226-move-domain-layer
  • Device: nanoSP or stax

📱 Mobile

-> Run Mobile E2E

  • Select "Run workflow"
  • Branch: chore/aggregated-assets-LIVE-35226-move-domain-layer
  • Device: nanoX

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 39 out of 40 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Generated file
Suppressed comments (4)

domain/entity/aggregated-asset/src/index.ts:2

  • The domain/entity/README.md conventions require a schema.mock.ts mock factory file for entity packages (and it should not be re-exported from the barrel). This package currently defines schema.ts + schema.test.ts but has no schema.mock.ts, so consumers/tests won’t have the standard way to create valid fixtures.

Please add src/schema.mock.ts (e.g., makeCryptoAssetMeta() / makeCryptoAssetMetaId() helpers) following the entity conventions.

export * from "./schema";
export * from "./types";

domain/entity/interest-rate/src/index.ts:2

  • The domain/entity/README.md conventions require a schema.mock.ts mock factory file for entity packages (and it should not be re-exported from the barrel). This package currently defines schema.ts + schema.test.ts but has no schema.mock.ts, so downstream tests lack the standard fixture factory.

Please add src/schema.mock.ts (e.g., makeInterestRate() / makeApyType() helpers) consistent with other entity packages.

export * from "./schema";
export * from "./types";

domain/api/aggregated-assets/src/errors.ts:1

  • FetchBaseQueryError is only used as a TypeScript type (in the predicate return type). Importing it as a value can introduce an unnecessary runtime dependency edge in emitted JS.

Prefer a type-only import here.

import { FetchBaseQueryError } from "@reduxjs/toolkit/query";

domain/api/aggregated-assets/src/api.ts:7

  • FetchBaseQueryError, FetchBaseQueryMeta, and QueryReturnValue are only referenced in the collectAllByCategory return type, so they should be imported as types. Keeping them in the value import can add unnecessary runtime imports when this is built/transpiled.

Consider splitting the import into value vs type-only.

import {
  createApi,
  fetchBaseQuery,
  FetchBaseQueryError,
  FetchBaseQueryMeta,
  QueryReturnValue,
} from "@reduxjs/toolkit/query/react";

LucasWerey added a commit that referenced this pull request Aug 4, 2026
… api

Addresses @ysitbon's review on #20345: comments 4, 5, 6, 7, 8 and 9.

shared/api-services already exists on develop and its README explicitly names
this migration: 'api-aggregated-assets is a placeholder for DADA ... when it
migrates, its base belongs here as src/services/dada/ rather than as another
standalone createApi'. I had missed it because I checked before rebasing.

Adds services/dada with the endpoint-less base api, then the use-case package
adds its endpoints with injectEndpoints and its cache tag with
enhanceEndpoints({ addTagTypes }), per that README. reducerPath stays the frozen
literal 'assetsDataApi', so one reducer, one middleware and one cache slice
still serve every use case - which the hand-scanning cache selectors depend on.

services/dada carries no extraArgument contract yet, unlike its siblings: DADA
endpoints build absolute URLs and pick prod/staging per request from an
isStaging query arg, so the base query has nothing to own. Migrating that to
extraArgument drops isStaging and touches both apps' store config, so it is
left as a documented TODO rather than smuggled into a relocation.

Layout, per the review:

  index.ts is now export * over public modules only, matching the other three
  domain/api barrels instead of enumerating ~30 named exports.

  internals/ stops being exported and now means what it says. Split by real
  consumers rather than by where things happened to live: market.ts and
  pagination.ts move out (dadaIdToMarketId has 5 app call sites,
  PartialMarketItemResponse 4), while chunkCurrencyIds, deepMergeCryptoAssets,
  emptyAssetsData, collectAllByCategory, assertDadaApiUrl and allSettled have no
  external consumers and stay in.

  collectAllByCategory and emptyAssetsData move to internals with their own
  files; the two public category accessors move to accessors.ts; request
  building and the chunked page fetch move to requests.ts. api.ts is now just
  the endpoint definitions.

The two internal helpers' characterization tests move with them from dada-client
into the api package, unchanged apart from the relative import - they were the
only remaining consumers, so keeping them where they were would have forced the
helpers to stay public. Same 246 tests as before, redistributed: 229 in
dada-client, 17 in the api package.
Copilot AI review requested due to automatic review settings August 4, 2026 11:43
Addresses @ysitbon's review on #20345: comments 4, 5, 6, 7, 8 and 9.

shared/api-services already exists on develop and its README explicitly names
this migration: 'api-aggregated-assets is a placeholder for DADA ... when it
migrates, its base belongs here as src/services/dada/ rather than as another
standalone createApi'. I had missed it because I checked before rebasing.

Adds services/dada with the endpoint-less base api, then the use-case package
adds its endpoints with injectEndpoints and its cache tag with
enhanceEndpoints({ addTagTypes }), per that README. reducerPath stays the frozen
literal 'assetsDataApi', so one reducer, one middleware and one cache slice
still serve every use case - which the hand-scanning cache selectors depend on.

services/dada carries no extraArgument contract yet, unlike its siblings: DADA
endpoints build absolute URLs and pick prod/staging per request from an
isStaging query arg, so the base query has nothing to own. Migrating that to
extraArgument drops isStaging and touches both apps' store config, so it is
left as a documented TODO rather than smuggled into a relocation.

Layout, per the review:

  index.ts is now export * over public modules only, matching the other three
  domain/api barrels instead of enumerating ~30 named exports.

  internals/ stops being exported and now means what it says. Split by real
  consumers rather than by where things happened to live: market.ts and
  pagination.ts move out (dadaIdToMarketId has 5 app call sites,
  PartialMarketItemResponse 4), while chunkCurrencyIds, deepMergeCryptoAssets,
  emptyAssetsData, collectAllByCategory, assertDadaApiUrl and allSettled have no
  external consumers and stay in.

  collectAllByCategory and emptyAssetsData move to internals with their own
  files; the two public category accessors move to accessors.ts; request
  building and the chunked page fetch move to requests.ts. api.ts is now just
  the endpoint definitions.

The two internal helpers' characterization tests move with them from dada-client
into the api package, unchanged apart from the relative import - they were the
only remaining consumers, so keeping them where they were would have forced the
helpers to stay public. Same 246 tests as before, redistributed: 229 in
dada-client, 17 in the api package.
@LucasWerey
LucasWerey force-pushed the chore/aggregated-assets-LIVE-35226-move-domain-layer branch from 61a9427 to d4f207e Compare August 4, 2026 11:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 49 out of 51 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Generated file
Suppressed comments (1)

shared/api-services/src/services/dada/api.test.ts:18

  • The test name/intent doesn’t match what’s being asserted: this doesn’t verify that the service declares no tag types (it only checks that a util thunk exists, which is true regardless of tagTypes). This is misleading and can mask regressions.

Consider either renaming the test to match the assertion, or replacing the assertion with a real check of the tag-type configuration (if there’s no public runtime API for that, renaming is the safer option).

Copilot AI review requested due to automatic review settings August 4, 2026 11:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 49 out of 51 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Generated file
Suppressed comments (2)

domain/api/aggregated-assets/src/index.ts:10

  • chunkCurrencyIds is implemented under src/internals/, but it is not exported from the package entrypoint. With the exports map only exposing . (and mocks), downstream packages can’t import it via a subpath even though the PR description calls out this helper as publicly needed by the platform layer. Either export it from src/index.ts (recommended) or adjust the stated public surface/plan.
export * from "./api";
export * from "./errors";
export * from "./market";
export * from "./pagination";

shared/api-services/src/services/dada/api.test.ts:18

  • This test name claims it checks tagTypes, but the assertion only checks that a util helper exists. As written, it doesn’t verify anything about tags and is misleading for future readers.

mergeAssetsDataPages was in the api package but the api never calls it - its
only consumers are useAssetsData and useStocksData, both hooks. Merging the
pages of an infinite query is a consumption decision, not something the api
does, so it belongs with the hooks in @features/platform-aggregated-assets.

It was also misplaced twice: its test was still in dada-client importing through
the shim, the same code-away-from-test split just corrected for chunkCurrencyIds
and deepMergeCryptoAssets.

Both move to features/platform/aggregated-assets as pagination.ts and
pagination.test.ts, assertions unchanged. The platform package gains
@domain/api-aggregated-assets for AssetsDataWithPagination, which is the correct
downward dependency; live-common's shim now re-exports from the platform
package.

Still 246 tests: 216 in dada-client, 17 in the api package, 13 here.
Copilot AI review requested due to automatic review settings August 4, 2026 11:57
Completes the second half of @ysitbon's comment on api.ts:53 - 'could go on
internals and could be tested there'. The move landed in d4f207e but it was
still only exercised indirectly, through the chunked endpoint returning it for an
empty id list.

Pins the invariant that matters: it must return a fresh object every call. The
chunked lookup endpoint uses it as a reduce seed and then mutates the accumulator
in place, so a shared instance would leak merged assets between queries.

collectAllByCategory (comment api.ts:163) is left covered through its two public
accessors, which the existing suite already exercises across pages, on a failing
page and against an untrusted host - testing it directly would only duplicate
that.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 53 out of 55 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Generated file
Suppressed comments (2)

shared/api-services/src/services/dada/api.test.ts:18

  • The test named "declares no tag types of its own" doesn’t actually assert anything about tag types (it only checks a util thunk is defined, which will be true regardless). This makes the test misleading and doesn’t protect the intended invariant.
    domain/api/aggregated-assets/package.json:13
  • PR description mentions that internals/ is exported publicly, but this package’s exports map only exposes the root entrypoint and mock subpaths. With this exports map, consumers won’t be able to import ./internals/* (or any other subpath) even if those helpers are meant to be public.
  "exports": {
    ".": "./src/index.ts",
    "./mock": "./src/assetsData.mock.ts",
    "./mock/stocks": "./src/stocks.mock.ts",
    "./mock/stablecoins": "./src/stablecoins.mock.ts",
    "./package.json": "./package.json"

Copilot AI review requested due to automatic review settings August 4, 2026 12:02
features/platform/README.md prescribes components/ hooks/ helpers/ with only
files that fit no subdirectory at the root. mergeAssetsDataPages is a
cross-feature domain-aware helper, which is what helpers/ is for.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 53 out of 55 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Generated file
Suppressed comments (2)

shared/api-services/src/services/dada/api.test.ts:18

  • The test name/intent doesn’t match the assertion: this spec says it “declares no tag types of its own” but it only checks that an RTK Query util function exists. This is misleading and reduces the value of the test suite (it won’t fail if tag types are accidentally added). Consider renaming the test (or assert the intended tagTypes behavior).
    shared/api-services/src/services/index.ts:5
  • dada is now exported as a first-class service, but shared/api-services/README.md still lists only 4 services and has a note saying @domain/api-aggregated-assets is a placeholder for DADA. That documentation is now out of date and could mislead future service additions / store wiring.

Addresses @ysitbon's comments 1 and 2 on #20345, for consistency with the rest of
the domain layer.

  fetchAt     -> DateTimeIsoSchema
  currencyId  -> union of CryptoCurrencyIdSchema and TokenCurrencyIdSchema

Feasibility was the open question and it is cheaper than expected: 0 errors in
libs/ledger-live-common, 0 in both new packages, and no brand breakage in either
app. The many DADA fixtures are untyped object literals, so branding the fields
does not reach them. Only one file needed touching - pagination.test.ts, whose
inline rate fixtures now go through InterestRateSchema.parse.

currencyId is a union, not CryptoCurrencyIdSchema alone: DADA keys interestRates
by both crypto ids (ethereum) and token ids
(ethereum/erc20/usd_tether__erc20_), so either brand on its own would mislabel
half the data.

fetchAt now rejects a malformed timestamp, so the test asserting the previous
'we deliberately do not validate this' intent is inverted rather than kept. All
four timestamp shapes DADA actually sends are covered, including its 6-digit
fractional seconds.

No runtime behaviour changes: nothing calls InterestRateSchema.parse at the api
boundary yet, which is LIVE-35232.
Copilot AI review requested due to automatic review settings August 4, 2026 12:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 51 out of 53 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Generated file
Suppressed comments (2)

shared/api-services/src/services/dada/api.test.ts:18

  • This test name/comment says it verifies tag types, but the assertion only checks that getRunningQueriesThunk exists (which doesn’t say anything about tagTypes). This makes the test misleading and harder to maintain.

Suggestion: rename the test (and drop the comment) so it matches what’s actually asserted, or assert the tag-types behavior in a verifiable way.
domain/api/aggregated-assets/package.json:14

  • PR description says internals/ is exported publicly, but the package exports map currently exposes only the root entrypoint and mock subpaths. As-is, consumers can’t import internals via subpath exports (e.g. @domain/api-aggregated-assets/internals/chunkCurrencyIds), which will block the follow-up work described.

If the intent is to make specific internals public, add explicit subpath exports for them (at least chunkCurrencyIds). If the intent is to keep them private for now, the PR description should be updated accordingly.

  "exports": {
    ".": "./src/index.ts",
    "./mock": "./src/assetsData.mock.ts",
    "./mock/stocks": "./src/stocks.mock.ts",
    "./mock/stablecoins": "./src/stablecoins.mock.ts",
    "./package.json": "./package.json"
  },

Fixes the live-common typecheck. Two characterization fixtures still assigned
plain strings to InterestRate.currencyId and .fetchAt, which are branded as of
the previous commit.

Both now go through InterestRateSchema.parse, so the fixtures are valid by
construction rather than cast. One fetchAt literal was '2026-07-31', which is not
RFC 3339 and would not have parsed - corrected to '2026-07-31T00:00:00Z'.

Missed locally because CI runs 'tsc --noEmit -p src/tsconfig.json
--customConditions node' while I had checked the root tsconfig, which does not
include these test files.
Copilot AI review requested due to automatic review settings August 4, 2026 12:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 53 out of 55 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Generated file
Suppressed comments (1)

shared/api-services/src/services/dada/api.test.ts:18

  • Test name doesn’t match what is asserted: this block is titled as if it validates tagTypes, but it only checks that a util thunk exists. This is misleading and makes it harder to understand what contract is actually being pinned.

The branded ids and RFC 3339 fetchAt forced three characterization
fixtures to be rebuilt through InterestRateSchema.parse, which breaks the
guarantee that those tests pass unmodified through the migration.

Revert to the structural z.string() shapes so the fixtures stay byte
identical to develop. Branding lands in LIVE-35232 alongside assetsIds,
where retargeting consumers is already in scope.
Copilot AI review requested due to automatic review settings August 4, 2026 12:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 51 out of 53 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Generated file
Suppressed comments (2)

shared/api-services/src/services/dada/api.test.ts:18

  • The test name says it validates tagTypes, but the assertion only checks that an RTK Query util helper exists (which is unrelated and would pass even if tagTypes were non-empty). Either assert something meaningful about tagTypes or rename the test to match what it actually verifies.
    shared/api-services/src/services/dada/constants.ts:9
  • This comment is much more detailed than other reducer-path constants in shared/api-services and is likely to go stale. Consider keeping it concise while still capturing the key constraint (the value is frozen and consumers scan/preload this key).

@sonarqubecloud

sonarqubecloud Bot commented Aug 4, 2026

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
27.5% Coverage on New Code (required ≥ 80%)
4 New Code Smells (required ≤ 1)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@LucasWerey

Copy link
Copy Markdown
Member Author

Quality Gate Failed Quality Gate failed

Failed conditions 27.5% Coverage on New Code (required ≥ 80%) 4 New Code Smells (required ≤ 1)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Won't be addressed in this PR

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If just for package test move it to a fixtures dir

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

common Has changes in live-common

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants